Must-fix

1. "One breaker signal per export" is only true for 429. WithSkipRateLimitRetry(ctx) in exporter.go suppresses retryablehttp's 429 retries, but 502/503/5xx still retry internally per
   isRetryableServerResponse in internal/client/client.go. A 503 storm will go through N internal retries before surfacing one error to the breaker — same problem the PR claims to fix, just one code path
   narrower. Either pass a stricter "skip all retries" context flag from telemetry, or accept the asymmetry and document it explicitly.
2. Retry-After is honored twice on 429. retryablehttp's backoff() (client.go ~L749) already waits Retry-After before re-attempting; the exporter then calls extendOpenStateAtLeast(...) with the same value, so
   the breaker sleeps Retry-After + waitDurationInOpenState. With both ~60s the effective pause is ~120s. Either move Retry-After handling exclusively to the breaker (don't retry at the transport layer for
   telemetry) or skip the breaker extension when retryablehttp owned the wait.
3. Retry-After not parsed on 503. Exporter only extracts the header when resp.StatusCode == 429. A 503 with Retry-After is silently ignored — breaker stays open the default 60s regardless of what the server
   asked for. Apply the same Retry-After parse to 503 (and 429), not just 429.

Should-fix

4. DSN backcompat is silent. telemetry_retry_count / telemetry_retry_delay are extracted and discarded (_, _ = params.extract(...) style). Users with legacy DSNs won't know their config is ignored. Log a
   one-time DEBUG (or WARN) when these params are present.
5. Breaker math change isn't documented. minimumNumberOfCalls 20→10 with FlushInterval 5s→30s means worst-case ~5 minutes to detect a sustained outage at default cadence. State this in a code comment so
   future maintainers don't accidentally regress it.
6. Sliding-window size didn't change with minimumNumberOfCalls. Window stayed at 30 while min dropped to 10. Verify intentional — the half-open recovery window now spans a smaller fraction, which shifts the
   trip-to-recover ratio.
7. buildUserAgent is duplicated, not extracted. PR body says it "mirrors internal/client/client.go:295-302 exactly" — two copies of the same logic will drift. Extract to a shared helper (e.g.,
   internal/agent/useragent.go) and have both call sites use it.

Test coverage

8. TestExport_SingleAttemptPerExport uses a bare *http.Client, not the retryablehttp-wrapped one. That correctly proves the exporter layer doesn't loop, but doesn't catch issue Add License file #1 above (transport-layer
   retry on 503). Add an integration-flavor test that uses the real RetryableClient() and asserts the combined behavior matches "one breaker signal per export" for all status codes the PR cares about.
9. TestIsRetryableStatus was deleted, not relocated. isRetryableStatus moved from exporter to the transport layer (isRetryableServerResponse); the predicate test should move with it into
   internal/client/client_test.go.
10. Breaker edge cases. New circuitbreaker_test.go (+118) covers happy paths for the Retry-After hint. Missing:
    - Two consecutive extendOpenStateAtLeast calls with different hints — which wins?
    - Half-open → fail → re-open while a prior hint is pending — does the hint persist? Reading the code, hints only clear on Close, so a stale hint can linger.

Thanks for the thorough review. All ten items addressed in a follow-up commit on this branch:

Must-fix

1. 5xx asymmetry fixed, not documented away. Renamed the context flag WithSkipRateLimitRetry → WithSkipTransientRetries and broadened it to suppress retries for both 429 and 503 (everything isRetryableServerResponse matches). Telemetry sets this, so the transport now collapses to exactly one HTTP transaction per export for 429, 503, 5xx, and transport errors — one breaker signal per call, no asymmetry. (internal/client/client.go, telemetry/exporter.go, telemetry/featureflag.go)

2. Double Retry-After eliminated. With Add License file #1 in place, retryablehttp never honours Retry-After internally on telemetry calls — the response flows directly back to the exporter. The exporter parses Retry-After once and feeds it to the breaker via extendOpenStateAtLeast. Also: the breaker already takes max(waitDurationInOpenState, retryAfterHint), not a sum, so no stacking even before this fix.

   While wiring this up I caught a related production bug: with retryablehttp's StandardClient adapter, when CheckRetry returns (false, err) net/http silently discards the response (logs "RoundTripper returned a response & error; ignoring response"). The exporter was therefore never seeing the actual 429/503 response in production. RetryPolicy now returns (false, nil) on the skip path so the response is preserved through net/http.

3. 503 Retry-After now honoured. Exporter parses Retry-After on 429 || 503 (was 429-only). Added TestExport_503RecordsRetryAfter to lock this in.

Should-fix
4. Legacy DSN params now logged. telemetry_retry_count / telemetry_retry_delay emit a one-time WARN on extraction noting they're deprecated and ignored. (internal/config/config.go)
5. Breaker timing documented. Expanded the comment on defaultCircuitBreakerConfig to spell out the minimumNumberOfCalls=10 × FlushInterval=30s = ~5min detection time, and why that's intentional (slow trip avoids flapping on transient blips for low-traffic clients; open-state interval owns the recovery cadence).
6. Sliding-window vs minimum calls clarified. Same comment: minimumNumberOfCalls gates when evaluation starts; slidingWindowSize caps how many outcomes participate once it does. Intentional — recent successes get to offset transient failures inside a larger memory window.
7. buildUserAgent deduplicated. Extracted to internal/client/useragent.go as client.BuildUserAgent(cfg). Both connector.go (telemetry/feature-flag path) and client.InitThriftClient (Thrift path) call it now — single source of truth.

Test coverage
8. Integration test through real RetryableClient. Added TestExport_SingleAttemptThroughRetryableClient which routes the export through client.RetryableClient(cfg) with a noop authenticator and asserts exactly 1 attempt for 429, 503, and 500 — catches transport-layer retry leaks for every status code the PR cares about. This is what surfaced the response-discarding bug fixed in #2.
9. TestIsRetryableStatus relocated. Added TestIsRetryableServerResponse in internal/client/client_test.go against the new predicate. Note the predicate semantics narrowed (only 429 + 503 now — 500/502/504 take the generic 5xx branch governed by ClientMethod); the test reflects that.
10. Breaker edge cases covered.
- TestCircuitBreaker_RetryAfterHintTakesMax: consecutive extendOpenStateAtLeast calls — larger wins, smaller is ignored, non-positive is a no-op.
- TestCircuitBreaker_HintClearedBeforeHalfOpenFailRecycle: Open(hint=10s) → HalfOpen (hint cleared) → fail → Open. Verifies the second Open uses waitDurationInOpenState, not the stale 10s hint — i.e. no stale hint persistence on the recycle path.

All existing tests still pass; go vet ./... clean. Diff stat: 9 files changed, +336 / −107.

This comment was generated with GitHub MCP.